kvserver: use wrapper type for Store.mu.replicas #72430
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
erikgrinaker
approved these changes
Nov 4, 2021
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, 1 of 6 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker and @tbg)
pkg/kv/kvserver/store_replicas_by_rangeid.go, line 23 at r1 (raw file):
type rangeIDReplicaMap struct { m syncutil.IntMap
nit: consider calling this replicas or something -- m.m is a bit terse.
pkg/kv/kvserver/store_replicas_by_rangeid.go, line 52 at r1 (raw file):
// returned from the closure to stop iteration without an error // resulting from Range(). func (m *rangeIDReplicaMap) Range(f func(*Replica) error) error {
nit: looks like we're never returning an error from the closure, so I wonder if it'd be just as easy to return a bool here for consistency with sync.Map and avoid the errcheck appeasement.
This simplifies lots of callers and it will also make it easier to work on cockroachdb#72374, where this map will start containing more than one type as value. Release note: None
Release note: None
- avoid the struct wrapper (and thus the need to name the inner map) - don't return an error in Range() Release note: None
tbg
force-pushed
the
store-clarify-maps
branch
from
3853f67
to
2113763
Compare
|
TFTR! I addressed your comments. bors r=erikgrinaker |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This simplifies lots of callers and it will also make it easier to work
on #72374, where this map will start containing more than one type as
value.
Release note: None